Conversation
…ion checks - Introduced Linear format as a target option in the convert command. - Updated command-line argument parsing to include Linear-specific options (team, project, parent). - Enhanced validation to ensure Linear tracker requires an epic ID. - Updated relevant documentation and help messages to reflect new features. - Added tests to validate Linear tracker configuration and error handling.
|
@omer9564 is attempting to deploy a commit to the plgeek Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds an end-to-end Linear tracker: docs, CLI convert/run integration, config validation, a typed Linear API client, issue body builder/parser, tracker plugin registration and implementation, and extensive unit tests across these components. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as User/CLI
participant Convert as ConvertCommand
participant Client as RalphLinearClient
participant LinearAPI as Linear API
participant Tracker as LinearTrackerPlugin
CLI->>Convert: convert --to linear + args
Convert->>Client: createLinearClient(apiKey)
Convert->>Client: resolveTeam(teamKey)
Convert->>Client: resolveLabelIds(labels)
Convert->>Client: resolveProject(project)
Convert->>Client: resolveOrCreateParent(parent|PRD)
Convert->>Client: createIssue(childIssue)
Client->>LinearAPI: API requests (teams, labels, projects, issues)
LinearAPI-->>Client: responses
Client-->>Convert: created issue metadata
CLI->>Tracker: run --tracker linear --epic epicId
Tracker->>Client: getWorkflowStates(teamId)
Client->>LinearAPI: fetch states
LinearAPI-->>Client: states
Client-->>Tracker: states
Tracker->>Client: getChildIssues(epicId)
Client->>LinearAPI: fetch child issues (paged)
LinearAPI-->>Client: issues
Client-->>Tracker: issues
loop per issue
Tracker->>Client: getBlockingIssueIds(issueId)
Client->>LinearAPI: fetch relations
LinearAPI-->>Client: relations
Client-->>Tracker: blockingIds
Tracker->>Tracker: linearIssueToTask(issue, blockingIds)
end
Tracker-->>CLI: return TrackerTask[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…d state resolution - Fix createBlockingRelation to use correct Linear SDK semantics (issueId=blocker, relatedIssueId=blocked) so Linear UI shows correct blocking direction - Fix getBlockingIssueIds to use inverseRelations() to properly find issues blocking a given issue, also supporting relations created directly in Linear - Parallelize countCompletedChildren using Promise.all instead of sequential awaits - Add pagination note to resolveLabelIds
There was a problem hiding this comment.
Code Review - Linear Task Tracker
Nice work on this PR! The overall architecture is clean and well-structured — the separation into client.ts (API wrapper), body.ts (markdown serialization), and index.ts (tracker plugin) follows the existing tracker patterns well. Tests are thorough with 159 tests passing.
Bugs Fixed (pushed to your branch)
1. Blocking relation direction was inverted (client.ts)
createBlockingRelation(blockingId, blockedId)was passingissueId: blockedId, relatedIssueId: blockingId— but the Linear SDK semantics fortype: Blocksmeans "issueId blocks relatedIssueId", so this created the opposite relation.getBlockingIssueIdswas usingissue.relations()(returns relations where issue isissueId) instead ofissue.inverseRelations()(where issue isrelatedIssueId). This happened to compensate for the creation bug, but meant:- Linear UI showed wrong blocking direction
- Relations created correctly by Linear users wouldn't be detected
- Fix: Swapped parameters in create, switched to
inverseRelations()in read.
2. Sequential state resolution in countCompletedChildren (index.ts)
- Was awaiting each child's state serially in a for-loop. Parallelized with
Promise.all.
Notes for future consideration (not blockers)
getBlockingIssueIdsis called per-child-issue ingetTasks()(N+1 query pattern). Acceptable for now but worth batching if performance becomes an issue with large epics.resolveLabelIdsandresolveProjectfetch up to 250 items without pagination — fine for most workspaces, added a comment noting the limit.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #343 +/- ##
==========================================
- Coverage 47.89% 47.34% -0.56%
==========================================
Files 105 111 +6
Lines 34358 36430 +2072
==========================================
+ Hits 16457 17248 +791
- Misses 17901 19182 +1281
🚀 New features to boost your workflow:
|
- Add comprehensive client.test.ts covering resolveApiKey, error classification, RalphLinearClient methods (resolveTeam, getIssue, getChildIssues, createIssue, updateIssueState, addComment, createBlockingRelation, getWorkflowStates, findWorkflowState, getBlockingIssueIds, resolveLabelIds, resolveProject, validateConnection), and createLinearClient factory - Add getPrdContext tests to index.test.ts - Coverage: client.ts 0% → 95%, index.ts 90% → 100%
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/commands/convert.test.ts (1)
53-112: Add regression cases for missing Linear option values.Current tests don’t lock behaviour for
--team/--project/--parentwhen the value is omitted and the next token is another flag. That edge case is where parsing currently degrades.Suggested tests
describe('Linear-specific options', () => { + test('rejects --team without a value', () => { + const result = parseConvertArgs([ + '--to', 'linear', + '--team', '--project', 'Sprint 1', + 'input.md', + ]); + expect(result).toBeNull(); + }); + + test('rejects --project without a value', () => { + const result = parseConvertArgs([ + '--to', 'linear', + '--team', 'ENG', + '--project', '--parent', 'ENG-1', + 'input.md', + ]); + expect(result).toBeNull(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/convert.test.ts` around lines 53 - 112, Add regression tests around parseConvertArgs to assert it fails (returns null or validation error) when Linear-specific flags are provided without values or the next token is another flag: add cases for '--team' with missing value (e.g. ['--to','linear','--team','--project','input.md']), '--project' missing value (e.g. ['--to','linear','--project','--team','input.md']), and '--parent' missing value (including when the next token looks like a flag or starts with '-'); ensure each new test calls parseConvertArgs and expects null, and include both the linear-required check and non-linear behavior where appropriate to lock the parser behavior.src/commands/convert.ts (1)
700-705: Add a defensive guard forargs.teamin the exported Linear executor.Line 704 uses a non-null assertion. Because
executeLinearConversionis exported, direct callers can bypassparseConvertArgsand hit avoidable runtime errors.Proposed fix
export async function executeLinearConversion( parsed: import('../prd/parser.js').ParsedPrd, args: ConvertArgs ): Promise<void> { - const teamKey = args.team!; + if (!args.team) { + printError('Error: --team <team-key> is required for Linear conversion'); + process.exit(1); + } + const teamKey = args.team;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/convert.ts` around lines 700 - 705, The exported function executeLinearConversion uses a non-null assertion on args.team (teamKey = args.team!) which can cause runtime errors if callers bypass parseConvertArgs; add a defensive guard at the start of executeLinearConversion to validate args.team (e.g., check if falsy) and handle the case by throwing a clear Error or returning early with a logged message; update any error text to reference executeLinearConversion and the team argument so callers get a descriptive failure rather than a crash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/linear-tracker.md`:
- Around line 92-94: Update the fenced code block containing the expression
"coarse_priority = min(4, max(0, ralph_priority - 1))" to include a language
identifier (for example "text" or "python") after the opening ``` so the block
is typed (e.g., ```text or ```python) to satisfy MD040 and improve
rendering/tooling consistency; locate the block that wraps the "coarse_priority
= ..." line and add the language token to the opening fence.
In `@src/commands/convert.ts`:
- Around line 108-114: The CLI currently consumes the next arg for options like
'--team', '--project', and '--parent' (variables team, project, parent) without
validating it, so flags like '--team --project X' are mis-parsed; update the
argument parsing in src/commands/convert.ts to first check that args[i+1] exists
and does not start with '-' before assigning (and if invalid, emit a clear error
and exit/throw), apply the same validation to the other parsing block that
handles these flags (the second occurrence around the 140-145 range), and ensure
both parsing branches consistently validate and fail fast instead of assigning
another flag as the value.
In `@src/config/index.ts`:
- Around line 749-756: The current validation only checks config.epicId for
Linear but ignores when an epicId is supplied via tracker options; update the
Linear check to accept either config.epicId or config.tracker.options.epicId (or
trackers[].options.epicId if applicable) by resolving the effective epic id
before validation, and also normalize this value back into config.epicId inside
buildConfig() so session metadata is consistent; locate references to
config.tracker.plugin, config.epicId, config.tracker.options.epicId and the
buildConfig() function to implement the resolution and normalization.
In `@src/plugins/trackers/builtin/linear/body.ts`:
- Around line 81-99: splitSections currently splits on any "##" H2 which breaks
task descriptions that contain their own H2s; update splitSections to first
normalize CRLF to '\n' (body = body.replace(/\r\n/g, '\n')), then only treat
headings whose text matches the Ralph section names (e.g., "Description",
"Acceptance Criteria", "Test Plan" — use a Set like const RALPH_HEADINGS = new
Set(['Description','Acceptance Criteria','Test Plan']) and compare
case-insensitively) before starting a new section; keep the same Map logic and
keys but only call sections.set(...) when the regex match's heading is in
RALPH_HEADINGS (use heading = match[1].trim()) so other H2s are treated as
regular content.
In `@src/plugins/trackers/builtin/linear/index.ts`:
- Around line 240-245: getTask currently converts blockingUuids to identifiers
using this.issueIdMap (populated only in getTasks), which can drop dependencies
when cache is empty; update getTask so after calling
this.client.getBlockingIssueIds(issue.id) you resolve any missing identifiers by
fetching the referenced issues (e.g., call this.client.getIssue(uuid) or another
client method that returns the issue key/identifier), populate this.issueIdMap
with the resolved identifier, and then build blockingIdentifiers before calling
linearIssueToTask; ensure the fetches are awaited and handle missing/failed
fetches (skip or use uuid fallback) so dependsOn is never silently dropped.
- Around line 272-285: The code currently returns the first in-progress task
before applying ralphPriority sorting, causing non-deterministic selection when
multiple in-progress tasks exist; change the logic so you first sort the tasks
by ralphPriority (use metadata?.ralphPriority or DEFAULT_RALPH_PRIORITY as in
the existing comparator) and then select the first task with status ===
'in_progress' from the sorted list (if any), otherwise return the first sorted
task; update references in this block (tasks, DEFAULT_RALPH_PRIORITY,
metadata?.ralphPriority, and the in-progress selection) accordingly.
---
Nitpick comments:
In `@src/commands/convert.test.ts`:
- Around line 53-112: Add regression tests around parseConvertArgs to assert it
fails (returns null or validation error) when Linear-specific flags are provided
without values or the next token is another flag: add cases for '--team' with
missing value (e.g. ['--to','linear','--team','--project','input.md']),
'--project' missing value (e.g.
['--to','linear','--project','--team','input.md']), and '--parent' missing value
(including when the next token looks like a flag or starts with '-'); ensure
each new test calls parseConvertArgs and expects null, and include both the
linear-required check and non-linear behavior where appropriate to lock the
parser behavior.
In `@src/commands/convert.ts`:
- Around line 700-705: The exported function executeLinearConversion uses a
non-null assertion on args.team (teamKey = args.team!) which can cause runtime
errors if callers bypass parseConvertArgs; add a defensive guard at the start of
executeLinearConversion to validate args.team (e.g., check if falsy) and handle
the case by throwing a clear Error or returning early with a logged message;
update any error text to reference executeLinearConversion and the team argument
so callers get a descriptive failure rather than a crash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b124378-3d41-412f-a6c9-f283dc7e55f4
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
docs/linear-tracker.mdpackage.jsonsrc/commands/convert.test.tssrc/commands/convert.tssrc/commands/run.tsxsrc/config/index.test.tssrc/config/index.tssrc/plugins/trackers/builtin/index.tssrc/plugins/trackers/builtin/linear/body.test.tssrc/plugins/trackers/builtin/linear/body.tssrc/plugins/trackers/builtin/linear/client.tssrc/plugins/trackers/builtin/linear/index.test.tssrc/plugins/trackers/builtin/linear/index.ts
- docs: add language identifier to fenced code block (MD040) - convert: validate CLI flag values exist and aren't other flags - convert: replace non-null assertion with defensive guard in executeLinearConversion - config: accept epicId from tracker.options and normalize to top-level - body: restrict splitSections to known Ralph headings so user H2s in descriptions are preserved; normalize CRLF - index: resolve missing identifiers in getTask via API fallback instead of silently dropping dependencies - index: sort by ralphPriority before selecting in_progress task for deterministic selection with multiple in-progress tasks - tests: add regression tests for missing flag values, H2-in-description, and CRLF handling
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/plugins/trackers/builtin/linear/client.test.ts (1)
339-363: Make the pagination mock cursor-sensitive.The second page is returned purely on call count, so this still passes if
getChildIssues()accidentally re-requests page 1 and never forwards the previous cursor. Having the mock assert the follow-up cursor would turn this into a real pagination regression test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/trackers/builtin/linear/client.test.ts` around lines 339 - 363, The pagination mock for mockSdkResponses.issue.children is currently based only on call count, which won't catch regressions where getChildIssues incorrectly re-requests the first page; update the mock to be cursor-sensitive by inspecting the incoming cursor argument and returning the first-page response only when cursor is undefined/null and the second-page response only when cursor === 'cursor-1' (matching pageInfo.endCursor), and add an assertion inside the mock to fail if an unexpected cursor is passed; this targets the mock used in the test for getChildIssues and ensures pageInfo.endCursor and subsequent calls are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/trackers/builtin/linear/client.test.ts`:
- Around line 139-154: The local helper function afterEach(fn: () => void) is
shadowing the test runner's afterEach and discarding the cleanup, so remove that
helper and restore the real teardown; change the file to rely on the test
framework's afterEach (or use beforeEach) to reset process.env.LINEAR_API_KEY,
or wrap each test's env mutation in try/finally to always restore originalEnv;
specifically remove the local afterEach definition near
describe('resolveApiKey') and keep the existing restore block that uses
originalEnv so the env is reliably restored even if a test throws.
In `@src/plugins/trackers/builtin/linear/index.test.ts`:
- Around line 359-421: The regression is that getTask(id) maps blocker UUIDs via
this.issueIdMap but never ensures the map is built unless getTasks() ran first;
update getTask (in src/plugins/trackers/builtin/linear/index.ts) to ensure the
issueIdMap is populated/rebuilt before mapping blockers: call the same
child-issue loading logic used by getTasks() (or extract that logic to a helper
like buildIssueIdMap and invoke it from both getTasks() and getTask()), then map
blocking UUIDs through this.issueIdMap to identifiers (ignoring unknown UUIDs)
so standalone calls to getTask('ENG-11') correctly set dependsOn.
---
Nitpick comments:
In `@src/plugins/trackers/builtin/linear/client.test.ts`:
- Around line 339-363: The pagination mock for mockSdkResponses.issue.children
is currently based only on call count, which won't catch regressions where
getChildIssues incorrectly re-requests the first page; update the mock to be
cursor-sensitive by inspecting the incoming cursor argument and returning the
first-page response only when cursor is undefined/null and the second-page
response only when cursor === 'cursor-1' (matching pageInfo.endCursor), and add
an assertion inside the mock to fail if an unexpected cursor is passed; this
targets the mock used in the test for getChildIssues and ensures
pageInfo.endCursor and subsequent calls are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 016b28e4-6790-4f62-833f-cc7b942e42a9
📒 Files selected for processing (2)
src/plugins/trackers/builtin/linear/client.test.tssrc/plugins/trackers/builtin/linear/index.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/plugins/trackers/builtin/linear/index.ts (2)
463-472: Workflow states cache may become stale if team changes within the same plugin instance.The cache invalidation at line 464 checks
this.teamId === teamId, butthis.teamIdis updated after fetching new states (line 469). This means:
- First call with team A: cache miss → fetch → set
this.teamId = A- Second call with team B: condition
this.teamId === teamIdis false (A !== B) → fetch → setthis.teamId = B- Third call with team A: condition is false (B !== A) → unnecessary refetch
While functionally correct (no stale data served), there's a minor inefficiency. Consider caching per team ID using a Map instead if multiple teams are expected.
💡 Optional: Per-team caching
- /** Cache of workflow states per team to avoid repeated API calls. */ - private workflowStatesCache: WorkflowStateSummary[] | null = null; + /** Cache of workflow states per team to avoid repeated API calls. */ + private workflowStatesCache = new Map<string, WorkflowStateSummary[]>(); private async getWorkflowStates(teamId: string): Promise<WorkflowStateSummary[]> { - if (this.workflowStatesCache && this.teamId === teamId) { - return this.workflowStatesCache; + const cached = this.workflowStatesCache.get(teamId); + if (cached) { + return cached; } const states = await this.client.getWorkflowStates(teamId); - this.teamId = teamId; - this.workflowStatesCache = states; + this.workflowStatesCache.set(teamId, states); return states; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/trackers/builtin/linear/index.ts` around lines 463 - 472, The current getWorkflowStates method uses a single this.workflowStatesCache and this.teamId, causing unnecessary refetches when the plugin serves multiple teams; replace the single cache with a per-team Map (e.g., workflowStatesCache: Map<string, WorkflowStateSummary[]>) and update getWorkflowStates to check workflowStatesCache.get(teamId) before calling this.client.getWorkflowStates(teamId), and then store the fetched states with workflowStatesCache.set(teamId, states); adjust any usages of this.workflowStatesCache and remove or repurpose the single this.teamId field accordingly so caching is keyed by teamId.
149-149: Consider adding a defensive guard before usingclientin public methods.The definite assignment assertion (
!) assumesclientis always initialised before use. Whileinitialize()setsready = falseon failure, the public methods (getTasks,getTask, etc.) don't checkisReady()before accessingthis.client.If a caller invokes these methods without checking
isReady()first, they'll encounter a runtime error on the uninitialised client.🛡️ Optional defensive guard pattern
override async getTasks(filter?: TaskFilter): Promise<TrackerTask[]> { + if (!this.ready) { + return []; + } const parentId = filter?.parentId ?? this.epicId;Or centralise the check in a helper:
private ensureReady(): void { if (!this.ready) { throw new Error('LinearTrackerPlugin is not initialised'); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/trackers/builtin/linear/index.ts` at line 149, Public methods like getTasks and getTask access the definite-assigned field client which may be uninitialised if initialize() failed; add a defensive guard before any use of this.client by either (A) introducing a private ensureReady() that throws if !this.ready and calling ensureReady() at the start of every public method (getTasks, getTask, etc.), or (B) make client possibly undefined and check if (this.client == null) { throw new Error('LinearTrackerPlugin is not initialised'); } in those methods; reference the existing ready flag and initialize() to keep semantics consistent.src/commands/convert.ts (1)
718-724: Clarify partial success behaviour in return value.The function returns
success: trueif any child issues were created (childIssues.length > 0), even if some failed. This is reasonable for a conversion tool, but the calling code at lines 840-843 exits with an error message "No child issues were created. Conversion failed." which only triggers on complete failure.Consider documenting this partial success behaviour or adding a warning when some stories failed to create.
💡 Optional: Add partial success warning
if (!result.success) { printError('No child issues were created. Conversion failed.'); process.exit(1); } + + if (result.childIssues.length < parsed.userStories.length) { + printInfo(`Warning: Only ${result.childIssues.length} of ${parsed.userStories.length} stories were created.`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/convert.ts` around lines 718 - 724, The current return sets success: childIssues.length > 0 which treats any partial creation as full success; change this to explicitly represent partial success by either (A) make success true only when all expected child issues were created (e.g. success = childIssues.length === expectedChildrenCount) and add a new field like partial = childIssues.length > 0 && childIssues.length < expectedChildrenCount, or (B) keep success as-is but add a warning flag/message when childIssues.length > 0 && childIssues.length < expectedChildrenCount so the caller can log a warning; reference the return object containing success, parentIssue, childIssues, relationsCreated and use whatever variable holds the expected count of children (e.g., the original stories/children array) to compute full vs partial success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/commands/convert.ts`:
- Around line 718-724: The current return sets success: childIssues.length > 0
which treats any partial creation as full success; change this to explicitly
represent partial success by either (A) make success true only when all expected
child issues were created (e.g. success = childIssues.length ===
expectedChildrenCount) and add a new field like partial = childIssues.length > 0
&& childIssues.length < expectedChildrenCount, or (B) keep success as-is but add
a warning flag/message when childIssues.length > 0 && childIssues.length <
expectedChildrenCount so the caller can log a warning; reference the return
object containing success, parentIssue, childIssues, relationsCreated and use
whatever variable holds the expected count of children (e.g., the original
stories/children array) to compute full vs partial success.
In `@src/plugins/trackers/builtin/linear/index.ts`:
- Around line 463-472: The current getWorkflowStates method uses a single
this.workflowStatesCache and this.teamId, causing unnecessary refetches when the
plugin serves multiple teams; replace the single cache with a per-team Map
(e.g., workflowStatesCache: Map<string, WorkflowStateSummary[]>) and update
getWorkflowStates to check workflowStatesCache.get(teamId) before calling
this.client.getWorkflowStates(teamId), and then store the fetched states with
workflowStatesCache.set(teamId, states); adjust any usages of
this.workflowStatesCache and remove or repurpose the single this.teamId field
accordingly so caching is keyed by teamId.
- Line 149: Public methods like getTasks and getTask access the
definite-assigned field client which may be uninitialised if initialize()
failed; add a defensive guard before any use of this.client by either (A)
introducing a private ensureReady() that throws if !this.ready and calling
ensureReady() at the start of every public method (getTasks, getTask, etc.), or
(B) make client possibly undefined and check if (this.client == null) { throw
new Error('LinearTrackerPlugin is not initialised'); } in those methods;
reference the existing ready flag and initialize() to keep semantics consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84dc5f99-5c41-4b0c-a25a-b9d627b597cc
📒 Files selected for processing (7)
docs/linear-tracker.mdsrc/commands/convert.test.tssrc/commands/convert.tssrc/config/index.tssrc/plugins/trackers/builtin/linear/body.test.tssrc/plugins/trackers/builtin/linear/body.tssrc/plugins/trackers/builtin/linear/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/plugins/trackers/builtin/linear/body.ts
- src/commands/convert.test.ts
… shadow, cursor-aware pagination mock
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/plugins/trackers/builtin/linear/index.ts (1)
482-485: Consider whether cancelled tasks should count as completed.The
countCompletedChildrenmethod includes both'completed'and'canceled'states in the count. Whilst this is reasonable for progress tracking (both represent "resolved" work), it may be surprising if users expectcompletedCountto exclude cancelled items.This is a minor semantic consideration and may well be intentional for your use case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/trackers/builtin/linear/index.ts` around lines 482 - 485, The countCompletedChildren method currently treats both 'completed' and 'canceled' Issue.state types as completed; decide the intended semantics and update the implementation accordingly: if cancelled tasks should not count toward completedCount, change the filter in countCompletedChildren to only include s?.type === 'completed'; if the current behavior is intentional, add a brief clarifying comment above countCompletedChildren explaining that 'canceled' is considered equivalent to 'completed' for progress metrics (or make this behavior configurable via a flag/option passed to the function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/plugins/trackers/builtin/linear/index.ts`:
- Around line 482-485: The countCompletedChildren method currently treats both
'completed' and 'canceled' Issue.state types as completed; decide the intended
semantics and update the implementation accordingly: if cancelled tasks should
not count toward completedCount, change the filter in countCompletedChildren to
only include s?.type === 'completed'; if the current behavior is intentional,
add a brief clarifying comment above countCompletedChildren explaining that
'canceled' is considered equivalent to 'completed' for progress metrics (or make
this behavior configurable via a flag/option passed to the function).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d35bf157-c21b-497c-88db-211eff13393c
📒 Files selected for processing (3)
src/plugins/trackers/builtin/linear/client.test.tssrc/plugins/trackers/builtin/linear/index.test.tssrc/plugins/trackers/builtin/linear/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugins/trackers/builtin/linear/client.test.ts
Linear client.test.ts and index.test.ts use mock.module() for different modules (@linear/sdk vs ./client.js) so they run in separate processes. body.test.ts and convert.test.ts are pure and batch together safely.
|
Addressed remaining review comments. Thanks for the PR @omer9564 🙌 |
Summary by CodeRabbit
New Features
Documentation
CLI
Tests
Chores